Simplify state held by AbstractSearchAsyncAction a little further #127145
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Continuing to simplify this a little further: No need to create the throttling CHM unless we actually are throttling, we can use the `null` to signal yes/no here. Likewise, no need to have an extra mutex field on the failures reference, we can use the set-once reference. Lastly, push down the building of the PIT id to the single place where it's actually used, that way we also save the transport version field that's only used in the PIT action on the parent class.
original-brownbear
added
>non-issue
:Search Foundations/Search
elasticsearchmachine
added
Team:Search Foundations
Collaborator
|
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
benchaplin
reviewed
Apr 22, 2025
|
|
||
| protected final void performPhaseOnShard(final int shardIndex, final SearchShardIterator shardIt, final SearchShardTarget shard) { | ||
| if (throttleConcurrentRequests) { | ||
| var pendingExecutionsPerNode = this.pendingExecutionsPerNode; |
Contributor
There was a problem hiding this comment.
Why assign this to a local variable?
Contributor
Author
There was a problem hiding this comment.
Still faster than repeatedly accessing a field https://openjdk.org/jeps/8349536 :) Doesn't matter too much here, but I had a follow-up planned where some of these fields stop being final and get nulled out. I code up a bigger change and then break pieces out of it for review :) and while it's sort of an irrelelvant line here it avoids some git-history-churn and a trivial amount of CPU/memory work doing it this way I guess :D
original-brownbear
added
v8.19.0
auto-backport
Contributor
Author
|
Thanks Ben! |
original-brownbear
deleted the
simplify-state-abstract-search-async-action
branch
Collaborator
💔 Backport failed
You can use sqren/backport to manually backport by running |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Continuing to simplify this a little further:
No need to create the throttling CHM unless we actually are throttling, we can use the
nullto signal yes/no here.Likewise, no need to have an extra mutex field on the failures reference, we can use the set-once reference.
Lastly, push down the building of the PIT id to the single place where it's actually used, that way we also save the transport version field that's only used in the PIT action on the parent class.